Let pushColorFilter accept all types of ColorFilters #9641
Conversation
|
This has at least one bug in it right now that I'm working on... crashes when you pass it a matrix. |
|
This should be good now. Will have tests upstream in framework. |
lib/ui/compositing.dart
Outdated
| assert(_debugPushLayer(layer)); | ||
| return layer; | ||
| } | ||
| EngineLayer _pushColorFilter(int color, int blendMode) native 'SceneBuilder_pushColorFilter'; |
There was a problem hiding this comment.
Instead of implementing 4 pushColorFilter variants, maybe it's cleaner to define a single pushColorFilter that takes a sk_sp<SkColorFilter>, and expose ExtractColorFilter from paint.cc to generate the SkColorFilter.
There was a problem hiding this comment.
My concern with that is the Paint version is a bit hacky and was mainly done to maintain backwards compatibility.
Paint can afford to do this because it already encodes/serializes this data to the engine side. Doing that is not free. I'd rather use this approach because it avoids allocating unnecessary typed data (which is demonstrably slow), and avoids sending data over to thenative side that we don't need.
There was a problem hiding this comment.
Thanks for pointing that out! I didn't realize that ColorFilter doesn't have a corresponding native SkColorFilter object. Is it possible to create such a native object during the construction of ColorFilter, so Paint can just store it inside _objects (like how Shader is stored), and pushColorFilter can also directly use it without triggering additional Dart to native conversion costs?
I guess that I'm Ok with having 4 pushColorFilter*** in scene_builder.cc but it just feels a little weird as every previous push*** in scene_builder.cc has a one-to-one relationship with the SceneBuilder.push*** in compositing.dart. I'll consult @chinmaygarde and @yjbanov to see how they feel about 4 pushColorFilter*** variants in scene_builder.cc.
There was a problem hiding this comment.
That may be a better way to go - just make ColorFilter a NativeFieldWrapper2 class. Do you want that to be part of this pull request or should we split it out?
There was a problem hiding this comment.
Let's have it here if it's not complicating this PR too much :) (I was hoping that it could even simplify this PR a little bit.)
There was a problem hiding this comment.
I split that effort out into #9668 which has landed and been merged into this PR. PTAL.
testing/dart/compositing_test.dart
Outdated
| }); | ||
| testNoSharing((SceneBuilder builder, EngineLayer oldLayer) { | ||
| return builder.pushColorFilter(const Color.fromARGB(0, 0, 0, 0), BlendMode.color, oldLayer: oldLayer); | ||
| return builder.pushColorFilter( |
There was a problem hiding this comment.
Let's add test coverage for all 4 types of ColorFilters. I think they don't have to be put in the SceneBuilder does not share a layer between addRetained and push* test. Maybe just add a new test pushColorFilter handles all 4 types?
dnfield
force-pushed
the
push_color_filter
branch
from
a226e13 to
882ca85
Compare
dnfield
force-pushed
the
push_color_filter
branch
from
882ca85 to
75a88a5
Compare
3 participants
Fixes flutter/flutter#35415
This allows consumers to apply all supported types of color filters to a scene.
This is a breaking change, but we don't actually use this code anywhere in the framework so I expect this code to pass.
Breaking change announcement: https://groups.google.com/forum/#!topic/flutter-announce/bdhRbfwg6jA